Add a Time singleton #49123

aaronfranke · 2021-05-27T02:03:08Z

Note: This PR will fail the CI checks because @qarmin's test project uses the methods from OS. There is nothing that I can do about this, the test project will simply have to be updated after this PR is merged.

Implements and closes #37800, implements and closes godotengine/godot-proposals#2114.

This PR adds a new Time singleton for working with time. Some of these methods are moved out of _OS in core_bind (which is the same as the exposed OS but is not the same as the internal OS), and some are new methods (including those requested by proposal 2114).

Compared to the current master, this code:

Is a better abstraction from a user's perspective, since time isn't really related to operating systems.
Is a lot cleaner overall. Things are more clearly named, there are more comments, parameters start with p_ now, etc.
Has better documentation, with clear statements of what standards are followed and what is/isn't supported.
Has better error messages for when invalid values are passed.
Has many test cases, vs the current code which doesn't have any test cases.
Can handle arbitrarily large times, supports up to billions of years in the past or future.
Has more features, especially converting with ISO 8601 date and time strings.

There are many new methods added by this PR to suit many use cases. They handle 3 formats: the Unix timestamp, a datetime dictionary, and ISO 8601 date and time strings (without timezone, as it's not stored in the Unix timestamp). Not all are strictly necessary as they can be built from the others, but not nearly as efficiently (going from dictionary -> Unix timestamp -> string and vice versa requires calculating the Unix timestamp, going from Unix timestamp -> dictionary -> string and vice versa requires constructing a dictionary, and going from Unix timestamp -> string -> dictionary and vice versa requires constructing and decomposing a string and doing extra calculations to find the weekday if that's desired). Some can be removed if desired, but I suggest keeping them all as there is very little code duplication - the heavy lifting is abstracted to macros at the top of time.cpp.

In addition to converting time, several of the methods are for getting time information from the operating system, in which case they call OS - the internal one, the previously exposed methods were in _OS. Remember that these methods were already "duplicated" before (_OS and internal OS), they've just moved (so it's now Time and internal OS).

Open question: Timezones and DST are not handled. Should they be? It would add complication to the code and require the user to supply more information: what the timezone is, whether DST is active (can't have an "auto" because different countries have DST at different times, except it is possible only when getting the current time from the OS but this part is already handled), and whether the time should be converted back/forth/or not. It might be simpler for the end user to require users to manually add/subtract/append/trim the timezone before/after converting as necessary.

Open question: Every single method starts with get_. It might make sense to just exclude this prefix.

Testing and feedback would be appreciated.

CsloudX · 2021-05-27T02:35:22Z

I think should a Date, a Time and a DateTime class. like Qt
All class has >,<， == etc operators.
All class has a current() method.
All class not a singleton.

This classes very useful for me.

aaronfranke · 2021-05-27T02:39:48Z

@CsloudX Definitely worth considering, as that's kinda what JS and Qt do. I don't think we need 3 of them though.

However, for comparison purposes, you can just use the Unix timestamp which is an int. <, >, ==, etc works. For getting the current time, there is Time.get_unix_time_from_system() (in 3.x it's OS.get_unix_time).

LikeLakers2 · 2021-05-27T03:38:13Z

Open question: Timezones and DST are not handled. Should they be? It would add complication to the code and require the user to supply more information: what the timezone is, whether DST is active (can't have an "auto" because different countries have DST at different times, except it is possible only when getting the current time from the OS but this part is already handled), and whether the time should be converted back/forth/or not. It might be simpler for the end user to require them to add/subtract/append/trim the timezone before/after converting as necessary.

Timezones are hard, so we should probably let the OS handle that if at all possible. Otherwise, I would let the user handle it -- or alternatively, don't handle it at all.

EricEzaM · 2021-05-27T07:12:15Z

Haha, I knew that was the Tom Scott video before clicking it.

I don't love how this uses so many massive macros. They could all easily be converted to methods which return a struct with year, month, day, h, m, s, etc, or int (i.e. in YMD_TO_DAY_NUMBER) which would help readability IMO.

aaronfranke · 2021-05-27T07:38:33Z

@EricEzaM That would indeed make sense for some of them like YMD_TO_DAY_NUMBER and VALIDATE_YMDHMS, I think I got carried a way a bit with macros, I'll change it if others agree but as-is it's pretty elegant because we don't even need to pass a list of arguments. However, for the ones that need to return multiple fields I don't really want to make so many structs that will only be used a few times. For EXTRACT_FROM_DICTIONARY the code to extract the information from the struct isn't too much of an improvement over extracting from the dictionary manually.

qarmin · 2021-05-27T09:05:17Z

I added workaround to CI, so it should pass now without any negative impact of this changes.

dalexeev · 2021-06-01T10:05:34Z

I think there should be a DateTime class (extends Reference) to work with datetime instances like in many other languages.

Calinou · 2021-06-01T14:30:15Z

I think there should be a DateTime class (extends Reference) to work with datetime instances like in many other languages.

I think it's better to leave this to a separate PR (and proposal). Otherwise, we risk making this PR huge and rejected in the end.

akien-mga · 2021-06-12T20:55:29Z

Thanks!

AndreaCatania · 2021-06-26T15:10:55Z

Looking at the function uint64_t get_ticks_msec() const; I've noticed that it's type was changed from uint32_t to uint64_t, there was any issue or why it's more preferable to use 64 bits in this case?

Calinou · 2021-06-26T15:14:36Z

Looking at the function uint64_t get_ticks_msec() const; I've noticed that it's type was changed from uint32_t to uint64_t, there was any issue or why it's more preferable to use 64 bits in this case?

This was likely done to prevent an overflow if you have a Godot process that runs for more than ~49.7 days (which is possible with a dedicated server).

aaronfranke added enhancement breaks compat labels May 27, 2021

aaronfranke added this to the 4.0 milestone May 27, 2021

aaronfranke requested a review from akien-mga May 27, 2021 02:03

aaronfranke requested review from a team as code owners May 27, 2021 02:03

aaronfranke added discussion needs testing labels May 27, 2021

aaronfranke force-pushed the it-is-time branch 2 times, most recently from 1c792ed to 91a68f8 Compare June 9, 2021 18:27

Add Time singleton

f64fea1

aaronfranke force-pushed the it-is-time branch from 91a68f8 to f64fea1 Compare June 11, 2021 13:42

akien-mga approved these changes Jun 12, 2021

View reviewed changes

akien-mga merged commit ac73059 into godotengine:master Jun 12, 2021

aaronfranke deleted the it-is-time branch June 12, 2021 21:09

aaronfranke mentioned this pull request Oct 21, 2021

[3.x] Backport the Time singleton from master #54056

Merged

mhilbrunner mentioned this pull request Oct 26, 2021

OS.get_datetime_from_unix_time() does not return the same value as OS.get_datetime() #52951

Closed

aaronfranke mentioned this pull request Oct 28, 2021

Update and fix Time documentation #54340

Merged

aaronfranke mentioned this pull request Nov 25, 2021

Improve OS time api #39727

Closed

skyace65 mentioned this pull request Dec 29, 2021

Documentation for OS.get_time_zone_info() does not tell if the bias is in seconds or minutes, etc. godotengine/godot-docs#5062

Closed

This comment was marked as resolved.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Time singleton #49123

Add a Time singleton #49123

aaronfranke commented May 27, 2021 •

edited

Loading

CsloudX commented May 27, 2021 •

edited

Loading

aaronfranke commented May 27, 2021 •

edited

Loading

LikeLakers2 commented May 27, 2021

EricEzaM commented May 27, 2021

aaronfranke commented May 27, 2021 •

edited

Loading

qarmin commented May 27, 2021

dalexeev commented Jun 1, 2021

Calinou commented Jun 1, 2021 •

edited

Loading

akien-mga commented Jun 12, 2021

AndreaCatania commented Jun 26, 2021

Calinou commented Jun 26, 2021 •

edited

Loading

This comment was marked as resolved.

Add a Time singleton #49123

Add a Time singleton #49123

Conversation

aaronfranke commented May 27, 2021 • edited Loading

CsloudX commented May 27, 2021 • edited Loading

aaronfranke commented May 27, 2021 • edited Loading

LikeLakers2 commented May 27, 2021

EricEzaM commented May 27, 2021

aaronfranke commented May 27, 2021 • edited Loading

qarmin commented May 27, 2021

dalexeev commented Jun 1, 2021

Calinou commented Jun 1, 2021 • edited Loading

akien-mga commented Jun 12, 2021

AndreaCatania commented Jun 26, 2021

Calinou commented Jun 26, 2021 • edited Loading

This comment was marked as resolved.

aaronfranke commented May 27, 2021 •

edited

Loading

CsloudX commented May 27, 2021 •

edited

Loading

aaronfranke commented May 27, 2021 •

edited

Loading

aaronfranke commented May 27, 2021 •

edited

Loading

Calinou commented Jun 1, 2021 •

edited

Loading

Calinou commented Jun 26, 2021 •

edited

Loading